Skip to content

Conversation

@guibescos
Copy link
Contributor

No description provided.

@guibescos guibescos marked this pull request as ready for review October 2, 2024 05:16
@vercel
Copy link

vercel bot commented Oct 2, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
api-reference ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 2, 2024 5:29am
proposals ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 2, 2024 5:29am
staking ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 2, 2024 5:29am

Copy link
Collaborator

@cprussin cprussin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but do we need reduce?

? options.stakeAccounts.split(",").map((m: string) => new PublicKey(m))
: [];

const instructions = stakeAccounts.reduce(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any reason to use reduce instead of map?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because StakeProgram.deactivate returns a list of instructions instead of a single instruction. In other words the type system doesn't guarantee that each element in the stakeAccounts list will map to a single instruction (although in practice StakeProgram.deactivate returns only one instruction I believe). Rather than return a list of list of instructions, this method will return a list that's the concatenation of all the instruction lists.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah got it, in general in those cases it's a bit more idiomatic to use flatMap than reduce

@guibescos guibescos merged commit f8ebeb6 into main Oct 3, 2024
6 checks passed
@guibescos guibescos deleted the deactivate-stake-comma-separated branch October 3, 2024 04:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants